As a general rule, embedding raw values directly into code expressions (of any language) is something to avoid. Here it happens to be safe because the value is always a hex SHA that won't include quotes.

With standalone jq we could pass it as --arg sha "$sha", but gh --jq doesn't support that so embedding is acceptable.

TL;DR: just a minor observation.

Yeah, I hate doing this. I spent a minute or so trying to think of a way to avoid it, and then decided that I’d rather rewrite it in GitHub Script if I’m going down that road anyway.

That said, given your point about the merge queue not allowing altering the commit message (which I had forgotten), I believe we could skip this loop and its API calls entirely and simply parse the PR numbers out from the commits. I can make that modification if you think it’s worthwhile. I am not sure this section of code has ever actually been used, though, since it only triggers when someone is active after the retirement PR is opened, before it the month passes, and before it’s manually closed.

I ended up doing this to save the API calls – though we have to inject the repository prefix anyway, and the script was previously doing something more dangerous there! But I just did it through the environment instead, so it’s all good. At least, as good as Bash can be.

Using || true here effectively hides any error from git log. The intent is to handle the BSD vs GNU head -1 discrepancy, an alternative that avoids masking real errors would be to use sed:

No, this is with GNU head(1), actually; the script has a dependency on GNU coreutils for date --date=…. It reproduces with both macOS and GNU head(1), and… also reproduces on Linux:

[emily@build01:~/nixpkgs]$ bash -c 'set -euo pipefail; foo=$(git log --reverse --first-parent --format=%cd --date=unix -- default.nix | head -1); echo hi'

[emily@build01:~/nixpkgs]$ 

So… I have no idea why it’s been working in CI. Maybe something to do with the Git configuration of the GHA runners, or terminal session signal handling, or something.

I believe sed -n 1p will let the git log command run to completion, so it’s less efficient in repositories with long histories. That shouldn’t matter here, of course.

However, this logic is actually incorrect when a committer is retired and then re‐added! It will find the first addition of the file, rather than the latest. So I’ve changed it to get the relevant information directly from Git instead.

--iso-8601=seconds already appends a timezone designator:

• GNU date ends with +00:00
• BSD date ends with Z

Manually adding another Z will therefore produce invalid timestamps such as 2025-01-01T12:00:00+00:00Z, so we should drop the extra suffix:

If you want full cross-platform consistency, using -u with a custom format avoids the GNU/BSD differences entirely:

Darwin date(1) doesn’t support --iso-8601 at all. I missed the time zone designator in the output and was trying to be paranoid in matching the documented format. However, it seems like the API parses numeric time zones correctly here, so I’ve just removed the Z as suggested.

A small FYI: gh api also supports {} path templates together with -f fields, which can help separate variable data from the path and avoid quoting/escaping pitfalls.

For example, this call could be written as:

gh api -X GET /repos/{org}/{repo}/commits/{sha}/pulls \
  -f org="$ORG" \
  -f repo="$ACTIVITY_REPO" \
  -f sha="$sha" \
  --jq '
    ...
  '

But the current style matches the rest of the file and is likely clearer for most readers, so no change needed.

Thanks! I do prefer that. But I’d prefer a proper programming language more, and it doesn’t seem worth refactoring the other calls for right now :)

It seems like we could also use -F for fancier type‐checking of fields (?).

It seems like we could also use -F for fancier type‐checking of fields (?).

Yeah, idk if it makes much difference for GET requests that use query params, but yes -F signals to gh that the value is a string. I think in some cases that'll lead to different escape sequences or quoting, though in practice, -f and -F seem to produce similar results.

We can avoid the issues with @lf- by using the GraphQL API:

#!/usr/bin/env bash
set -euo pipefail

user=$1

id=$(gh api graphql -f query='query($user: String!) { user(login: $user) { id } })' -f user="$user" --jq '.data.user.id')

if ! result=$(gh api graphql --paginate -f query="$(<commits.graphql)" -f id="$id" -f since="$(date -Iseconds --date='1 year ago')"); then
  echo "$result"
else
  jq . > result.json <<< "$result"
fi

gh api graphql --paginate -f query='
query($id: ID!, $since: GitTimestamp!, $endCursor: String) {
  repository(owner: "NixOS", name: "nixpkgs") {
    ref(qualifiedName: "master") {
      target {
        ... on Commit {
          history(since: $since, first: 100, author: { id: $id }, after: $endCursor) {
            nodes {
              parents(first: 2) {
                nodes {
                  oid
                }
              }
              signature {
                wasSignedByGitHub
              }
              committedDate
              url
            }
            pageInfo {
              hasNextPage
              endCursor
            }
          }
        }
      }
    }
  }
}' -f id="$id" -f since="$(date -Iseconds --date='1 year ago')" \
  --jq '.data.repository.ref.target.history.nodes[] | select((.parents.nodes | length > 1) and .signature.wasSignedByGitHub) | pick(.url, .committedDate)'

I'm out of time myself for now, but I can integrate this into the PR later.

Fyi: The gh CLI supports pagination both for the REST and GraphQL API's ;)

Since this isn’t the first time automation has had to special‐case @lf-, and some things are exposed only through the REST API, I’d much rather use our connection to GitHub to try and get this API bug fixed before considering increasing the code complexity like this.

I deliberately skipped pagination, since it’s extremely unlikely for someone to have 100 @web-flow commits through after their last PR merge, and it’d make the script run much slower in the presence of highly‐active committers. This GraphQL version makes that case much more likely, since it will pick up every commit authored by a committer.

Yeah, to be clear, when Emily iterated on this, we discussed things - and she already wrote a very similar GraphQL query, but the problem there is that it is impossible to filter by committer via GraphQL.

This does filter after the fact by the GitHub signature, which should correctly handle that, FWIW – but it also means having to enumerate every authored commit in the past year (since 100 non‐web commits authored after the last merge is much more plausible), which for active committers I think would significantly slow the script down. The commits endpoint is already somewhat slower than the activity one. So I don’t think this is a good idea just to avoid a bug we can report to GitHub that adds 1 PR/year additional triage load, even before code complexity is taken into account.

I reported this bug to GitHub and they told me they don't care to fix it. It's that they're using the current username regex while evaluating searches rather than one which matches their historical username requirements, and it's probably a 1-line fix. Their reply didn't really make sense to me as to why they didn't want to fix it.

You can try again if you'd like.

it’d make the script run much slower in the presence of highly‐active committers

We can stop the pagination once any valid merge has been found, which should always just need one page for active committers.

While I'm happy to maintain this repo even with added complexity to get it work nicely, it really does seem like this is a one-off case, especially considering what @lf- mentioned, so I guess this is fine.

@lf- Aha, interesting. I’m not sure if that’s the only thing going on here, though – Wolfgang pointed out that he has seen it happen with @mbrgm too, which presumably doesn’t have any name validation issues. Maybe the regex is for search endpoints and whatever is going on with the API here is something else – or maybe the issue with @mbrgm is unrelated (it seems they have some commits in master, but they don’t show up in /commits?author=… in the web interface either, whereas yours do).

I certainly think it’s worth asking GitHub about, since we have another call with them scheduled in a week anyway.

@infinisil

I tried the GraphQL snippet here with @fabaff, and it took almost 5 minutes for that one committer, so I think this approach is certainly a non‐starter without some kind of short‐circuiting. However there are subtleties to that too, when considering e.g. the “user has become inactive again” logic.

What would probably be more viable is doing something like git clone --bare --single-branch --no-tags --filter=tree:0 https://github.com/NixOS/nixpkgs.git, git log --first-parent to aggregate commits from the last year and filter by author (if the committer is GitHub) and otherwise committer, group by email address, take the first commit for each, and then use the API to map those commits to usernames; whatever is left are inactive committers. But I believe the current state of the PR is really fine and think it would not be a good return‐on‐investment to work on that.

Regarding feedback for GitHub, it seems like what's really missing is that the activity endpoint should keep track of merge queue additions. If we had that, we wouldn't need any changes.

Merge queue counting as activity would be good too and probably something we should raise, but I’m guessing it’d be a less trivial fix (it seems like much of the merge queue stuff happens through an internal bot account) – and we’d still need to make adjustments here to filter out non‐PR‐merging activity, so I’m not sure it’d be a net reduction in complexity.

We don't know how GitHub's backend works, so I don't think we can make realistic estimates for complexities of changes, neither for the username regex nor for the activity endpoint. What we can do is suggest multiple potential solutions to them, and see if they can do any of them.

For filtering the non-PR-merging activity, it would just be select(.activity_type == ...) in the jq.

I believe the merge queue events are visible in the activity results - just for the web-flow or other bot users. So it'd be a change that doesn't make any semantic sense for them: The user that pushes the head of the temporary merge queue branch to the target branch is the bot.

The right thing to do would be to look at the events where the PR is added to the merge queue. But these are not repository events, they are PR events. We can already fetch them for each PR, but we won't get them via the activity endpoint.

TLDR: I don't even think there is a reasonable change for GitHub to make - everything works as it should.